-
Notifications
You must be signed in to change notification settings - Fork 56
Fix handling of real tensors with complex scalartype #360
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Haven't managed to go through this in detail, but I wonder if before trying to fix this and make it consistent, it might be useful to first settle the question if we want to separate between |
|
I think in general I completely agree to introduce these functions and make that distinction. |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
|
|
|
@Jutho, I think I somewhat centralized determining the scalartype now, and in principle this is ready to start an attempt to make |
| domain(t) == codomain(t) || | ||
| throw(SpaceMismatch("Trace of a tensor only exist when domain == codomain")) | ||
| s = zero(scalartype(t)) | ||
| s = zero(scalartype(t)) * zero(dimscalartype(sectortype(t))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps dimscalartype(I) could have been included in computing scalartype? I don't think there is any case where fusionscalartype would be Int, but dimscalartype would be a floating point type, but I guess adding this here doesn't hurt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about that, I do really think about scalartype as returning the type of the data entries, (even if that does not sound right, this is definitely how we have been using it), so in principle I could conceive of having integer entries, but anyonic symmetry (although I don't know if there really is that much use for that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only way to have a unitary fusion category with integer F symbol is for Rep{G} or Vec{G} where G is an abelian group. Hence, in all other cases scalartype is at least a non-integer real type. But I am fine leaving this as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to double check, do you happen to mean sectorscalartype? Otherwise I'm slighty confused
Co-authored-by: Jutho <Jutho@users.noreply.github.com>
This is an attempt at being more principled about dealing with complex
sectorscalartypes with tensors that don't necessarily have complex entries.In the context of #356 I bumped into a couple of these, so I figure might as well look a bit more carefully at the various occurrences.
My general strategy here is to use
TO.tensoralloc_addas much as possible, since we already fixed this issue in that function.However, I also had to modify
compose_dest, since there are no recouplings required there, and therefore there is no need to update the scalartype.I also added the utility
check_spacetypefunction that does exactly what you suspect it does, and just centralizes various places in the code where this was in use.